Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DOC] Correctly mark public link-to component named arguments #19348

Merged
merged 1 commit into from
Feb 19, 2021
Merged

[DOC] Correctly mark public link-to component named arguments #19348

merged 1 commit into from
Feb 19, 2021

Conversation

bertdeblock
Copy link
Member

@@ -406,6 +406,18 @@ const LinkComponent = EmberComponent.extend({
**/
replace: false,

/**
Determines whether the `LinkComponent` will prevent the default
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the description of the Allowing Default Action section in the LinkTo API docs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, I wonder if this should just be deprecated and removed as part of RFC 707.

This seems only useful in the context of changing the tagName to something other than <a> (which is deprecated in RFC 707).

When the tagName is <a>, the default action of the browser is to navigate away, so there is no sense in not preventing it.

Thoughts?

Looping in @rwjblue as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I completely understand what you mean. I've never used @preventDefault myself, but I can imagine it being useful for some use cases to open a route in a new tab.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow sorry, scratch that. I was confused. Yeah I can't immediately imagine a scenario where a refresh is desired.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chancancode - ya seems correct to me

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chancancode - So... Merge now and update the RFC RE: deprecating preventDefault?

@chancancode
Copy link
Member

@rwjblue @bertdeblock How about we move the docs currently in the "Allowing Default Action" section into the documentation for the property itself, and mark the property as @private? That way the documentation can still technically be found if someone needs it but we avoid marking it as @public just to have it deprecated soon after.

Reading the docs again – I suppose there is case to be made for keeping this even if @tagName is deprecated. In theory, you may want a link to act like an "ember link" most of the time, except when certain condition is true, then you want it to actually trigger a full page reload. Something like this:

<LinkTo @route="dashboard" @preventDefault={{this.requiresRefresh}}>
  Return to dashboard
</LinkTo>

Where this.requiresRefresh is conditionally true, perhaps when the user is a new sign up that just finished the on boarding process, something like that.

If someone wants to make the case for it, I think it wouldn't cost much to keep the functionality around. Personally, I think it's not really needed, and there are probably better ways of accomplishing that without using <LinkTo>, so I am still leaning towards deprecating it, but I could go either way on that.

chancancode added a commit to emberjs/rfcs that referenced this pull request Jan 21, 2021
@chancancode
Copy link
Member

I opened a PR on the RFC repo to amend this if we want to go forward: emberjs/rfcs#710

@bertdeblock
Copy link
Member Author

This sounds good to me. I don't have any incentive to keep this functionality around, but of course maybe other people do. Let me know when / if I should update this PR to move the docs 👍.

@chancancode
Copy link
Member

I think you can go ahead, if we ended up wanting to make it public it's easy enough to change the tag.

@bertdeblock
Copy link
Member Author

Moved the docs (hopefully I understood it correctly). Let me know if anything.

Copy link
Member

@chancancode chancancode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue seems good to you?

@bertdeblock
Copy link
Member Author

Gentle ping. Anything left I should take care of?

Copy link
Member

@chancancode chancancode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay!

@chancancode chancancode merged commit 1866329 into emberjs:master Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants